mic_privacy: initial implementation #9788

mbukowsk · 2025-01-20T21:35:49Z

Audio privacy feature allows end user to directly
control if user space applications receive actual
data from input devices (microphones). The control
is bypassing application level settings or operating
system controls (like audio endpoint volume).

src/audio/base_fw.c

ujfalusi · 2025-01-21T09:17:18Z

src/include/ipc4/base_fw.h

-	IPC4_UAOL_CAPS_HW_CFG            = 10
+	IPC4_UAOL_CAPS_HW_CFG            = 10,
+	/* Mic privacy capabilities */
+	IPC4_PRIVACY_CAPS_HW_CFG         = 11


I would re-name these to be more specific to:
IPC4_INTEL_MIC_PRIVACY_CAPS_HW_CFG (and IPC4_INTEL_UAOL_CAPS_HW_CFG).

it looks to me that we are passing the raw content of the DfMICPVCP register, which is pretty much Intel specific

Can we rename this to IPC4_INTEL_MIC_PRIVACY_CAPS_HW_CFG. I don't think other vendors will have MICPVCP register in their SoC.

src/audio/base_fw.c

src/audio/copier/copier_generic.c

src/audio/copier/copier_hifi.c

app/boards/intel_adsp_ace30_ptl.conf

src/audio/copier/copier.c

src/audio/mic_privacy_manager/mic_privacy_manager.c

lyakh · 2025-01-21T09:26:19Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+		return mic_privacy_api->get_dma_data_zeroing_link_select();
+	}
+	//hardcoded for FW_MANAGED
+	return 0xFFFFFFFF;


lyakh · 2025-01-21T09:28:18Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+	return 0xFFFFFFFF;
+}
+
+struct mic_privacy_settings fill_mic_priv_settings(uint32_t mic_disable_status)


I think supplying a structure to be filled as a parameter is preferred over returning one

src/audio/mic_privacy_manager/mic_privacy_manager.c

lgirdwood

@mbukowsk can you confirm a high level overview of how it works when FW managed. i.e. I assume FW will send an IPC to SW when the mute is enabled/disabled ? What about Mic state at FW boot ?

src/audio/mic_privacy_manager/mic_privacy_manager.c

src/include/sof/audio/mic_privacy_manager.h

lgirdwood · 2025-01-21T14:01:26Z

src/include/sof/audio/mic_privacy_manager.h

+#include <ipc4/base-config.h>
+#include "../audio/copier/copier_gain.h"
+
+#define ADSP_RTC_FREQUENCY						 32768


This should be in Zephyr as its HW specific.

src/include/sof/audio/mic_privacy_manager.h

lgirdwood · 2025-01-21T14:04:15Z

src/include/sof/audio/mic_privacy_manager.h

+int mic_privacy_manager_init(void);
+
+int mic_privacy_manager_get_policy(void);
+
+uint32_t mic_privacy_get_policy_register(void);
+
+void enable_fw_managed_irq(bool enable_irq);
+
+void enable_dmic_irq(bool enable_irq);
+
+void handle_dmic_interrupt(void * const self, int a, int b);
+
+void handle_fw_managed_interrupt(void * const dev);
+
+void propagate_privacy_settings(struct mic_privacy_settings *mic_privacy_settings);
+
+uint32_t get_dma_zeroing_wait_time(void);
+
+uint32_t get_privacy_mask(void);
+
+struct mic_privacy_settings fill_mic_priv_settings(uint32_t mic_disable_status);
+
+void set_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t mic_disable_status);
+
+void update_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t hw_mic_disable_status);
+
+void mic_privacy_process(struct comp_dev *dev, struct mic_privacy_data *mic_priv, struct comp_buffer *buffer, uint32_t copy_samples);
+
+void mic_priv_gain_input(uint8_t *buff, uint32_t buff_size,  uint32_t mic_priv_state,
+					const struct ipc4_audio_format *in_fmt);
+


If these are all for client usage of Mic privacy then they should all share the same mic_privacy_ prefix.

src/lib/dma.c

xtos/include/sof/lib/dma.h

src/audio/base_fw.c

jsarha

No bugs from me (the others already found the serious stuff), but couple of suggestions more.

src/audio/copier/copier_dai.c

src/lib/dma.c

src/include/ipc4/base_fw.h

src/audio/copier/copier.c

lgirdwood · 2025-02-10T14:32:21Z

@mbukowsk can you check CI, several failures. Thanks !

ujfalusi

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

src/audio/copier/copier_gain.c

src/audio/copier/copier_gain.h

src/audio/base_fw.c

ujfalusi · 2025-02-10T15:14:36Z

src/audio/copier/copier.c

@@ -51,6 +54,10 @@ SOF_DEFINE_REG_UUID(copier);

 DECLARE_TR_CTX(copier_comp_tr, SOF_UUID(copier_uuid), LOG_LEVEL_INFO);

+#if CONFIG_MICROPHONE_PRIVACY
+static int mic_privacy_configure(struct comp_dev *dev, struct copier_data *cd);
+#endif


why don't you add the mic_privacy_configure() so that forward declaration is not needed?
Plus:

#else static inline int mic_privacy_configure(struct comp_dev *dev, struct copier_data *cd) { return 0 } #endif /* CONFIG_MICROPHONE_PRIVACY */

and you can save on the ifdefs in code

ujfalusi · 2025-02-10T15:15:05Z

src/audio/copier/copier.c

@@ -131,6 +138,15 @@ static int copier_init(struct processing_module *mod)
 				comp_err(dev, "unable to create host");
 				goto error;
 			}
+#if CONFIG_MICROPHONE_PRIVACY
+			if (cd->direction == SOF_IPC_STREAM_CAPTURE && node_id.f.dma_type == ipc4_hda_host_output_class) {


123 chars long..

still 123 long.

ujfalusi · 2025-02-10T15:20:16Z

src/audio/copier/copier_gain.c

@@ -27,7 +30,7 @@ int copier_gain_set_params(struct comp_dev *dev, struct copier_gain_params *gain
 	/* Set basic gain parameters */
 	copier_gain_set_basic_params(dev, gain_params, ipc4_cfg);

-	switch (dd->dai->type) {


how did this compiled before this patch?
I cannot see dd anywhere in the change,

and if there were dd to begin with, then why we don't we take the pointers out from it locally?

ujfalusi · 2025-02-10T15:22:06Z

src/audio/copier/copier_gain.h

 * @return 0 on success, negative error code on failure.
 */
-int copier_gain_set_params(struct comp_dev *dev, struct copier_gain_params *gain_params, uint32_t fade_period);


no dd was here either

ujfalusi · 2025-02-10T15:24:25Z

src/audio/copier/copier_gain.c

+		struct ipc4_copier_module_cfg *copier_cfg = cd->dd[0]->dai_spec_config;
+		const int channels = copier_cfg->base.audio_fmt.channels_count;
+
+		ret = copier_set_gain(dev, cd->dd[0]->gain_data, gain_data, channels);


two gain_data is a bit confusing here.

ujfalusi · 2025-02-10T15:30:16Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+	}
+}
+
+void update_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t hw_mic_disable_status)


I'm not sure, but I cannot see that this function is called?

I think this is still not called?

lgirdwood · 2025-02-10T16:10:39Z

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

@ujfalusi ok that's a blocker for kernel. Can you reply with what this should look like for kernel so @mbukowsk can copy.

ujfalusi · 2025-02-11T07:18:07Z

@mbukowsk, the privacy_capabilities is configured to the wrong struct, that is going to case the host quite a bit of issues as it will not find it in hw_config and there will be two entries with the same ID in fw_config.

@ujfalusi ok that's a blocker for kernel. Can you reply with what this should look like for kernel so @mbukowsk can copy.

No, this is a blocker for this PR.

The firmware places the IPC4_PRIVACY_CAPS_HW_CFG (as is a hardware configuration tuple with ID 11) into the FW_CFG (as is firmware configuration). The ID 11 is IPC4_MAX_ASTATE_COUNT_FW_CFG in firmware config.

These are different configurations, one is queried with SOF_IPC4_FW_PARAM_FW_CONFIG (basefw param 6), the other is SOF_IPC4_FW_PARAM_HW_CONFIG_GET (basefw param 7).

The IPC4_PRIVACY_CAPS_HW_CFG must be placed to the hardware config data in basefw_hw_config() and not in basefw_config() as this PR does.

The corresponding kernel patch is thesofproject/linux@7f00cf9

ujfalusi · 2025-02-11T07:24:12Z

src/audio/copier/copier_gain.c

 	int ret;

 	/* Set basic gain parameters */
-	copier_gain_set_basic_params(dev, dd, ipc4_cfg);
+	copier_gain_set_basic_params(dev, gain_params, ipc4_cfg);

 	switch (dd->dai->type) {


Right, so at this patch this is not going to compile. dd is not defined.

for dd it looks like part of this change is placed in the next commit, I will move it to the first one

looks that way, I'll wait for the update with the fix.

Change type of arguments for gain params setting functions. DAI data struct is replaced by gain_params. Components other than DAI can also use copier gain feature (eg. microphone privacy) Signed-off-by: Michal Bukowski <[email protected]>

Audio privacy feature allows end user to directly control if user space applications receive actual data from input devices (microphones). The control is bypassing application level settings or operating system controls (like audio endpoint volume). Signed-off-by: Michal Bukowski <[email protected]>

Added empty implementation which always returns success Signed-off-by: Michal Bukowski <[email protected]>

…ANGE SW sends this IPC when microphone privacy state is changed for HW_MANAGED mode and SNDW interface. Signed-off-by: Michal Bukowski <[email protected]>

ujfalusi

@abonislawski , there are also valid checkpatch finds.

ujfalusi · 2025-02-11T12:37:06Z

src/audio/copier/copier_gain.c

 	int ret;

 	/* Set basic gain parameters */
-	copier_gain_set_basic_params(dev, dd, ipc4_cfg);
+	copier_gain_set_basic_params(dev, gain_params, ipc4_cfg);

 	switch (dd->dai->type) {


looks that way, I'll wait for the update with the fix.

ujfalusi · 2025-02-11T12:39:14Z

src/audio/Kconfig

+	  Audio privacy feature allows end user to directly control if user space
+	  applications receive actual data from input devices (microphones).
+	  The control is bypassing application level settings or operating system
+	  controls (like audio endpoint volume).


This is really Intel specific implementation of a microphone privacy, I think this has to be mentioned

ujfalusi · 2025-02-11T12:39:32Z

src/audio/base_fw.c

@@ -25,6 +25,7 @@
 #include <sof/debug/telemetry/performance_monitor.h>
 #include <sof/debug/telemetry/telemetry.h>
 #include <sof/debug/telemetry/performance_monitor.h>
+


spurious change

ujfalusi · 2025-02-11T12:41:50Z

src/include/ipc4/base_fw.h

-	IPC4_UAOL_CAPS_HW_CFG            = 10
+	IPC4_UAOL_CAPS_HW_CFG            = 10,
+	/* Mic privacy capabilities */
+	IPC4_PRIVACY_CAPS_HW_CFG         = 11


Can we rename this to IPC4_INTEL_MIC_PRIVACY_CAPS_HW_CFG. I don't think other vendors will have MICPVCP register in their SoC.

ujfalusi · 2025-02-11T12:42:47Z

src/include/sof/audio/mic_privacy_manager.h

+
+uint32_t get_dma_zeroing_wait_time(void);
+
+uint32_t get_privacy_mask(void);


no need for these blank lines.

ujfalusi · 2025-02-11T12:49:58Z

src/audio/dai-zephyr.c

@@ -362,12 +363,12 @@ dai_dma_cb(struct dai_data *dd, struct comp_dev *dev, uint32_t bytes,
 					ret = -EINVAL;
 					continue;
 				}
-


the blank line was fine.

ujfalusi · 2025-02-11T12:50:16Z

src/audio/dai-zephyr.c

 									  sink, converter[j],
 									  bytes, dd->chmap);
+


no need for blank line

ujfalusi · 2025-02-11T12:52:02Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+
+LOG_MODULE_REGISTER(LOG_DOMAIN);
+
+int mic_privacy_manager_init(void)


this file is Intel specific, can we emphasize it with file name and external API names?

ujfalusi · 2025-02-11T12:55:36Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+			mic_priv_data->mic_privacy_state = MIC_PRIV_MUTED;
+		} else {
+			mic_priv_data->mic_privacy_state = MIC_PRIV_UNMUTED;
+		}


I'm not sure what is the coding style in SOF now-days, but this patch uses two style when there is a single instruction in the if/else
Zephyr demands curly brackets always, like this. We used to be following the kernel's with no need for them.

ujfalusi · 2025-02-11T12:56:59Z

src/audio/mic_privacy_manager/mic_privacy_manager.c

+	}
+}
+
+void update_gtw_mic_state(struct mic_privacy_data *mic_priv_data, uint32_t hw_mic_disable_status)


I think this is still not called?

lgirdwood

@abonislawski some high level questions.

In HW managed - FW gets IRQ from HW and mutes. Does it send IPC to SW to inform ?
In FW managed mode - How does FW know when to mute - is it relying on SW to send an IPC ?

mbukowsk requested review from kv2019i, lyakh, iuliana-prodan, dbaluta, abonislawski, RanderWang, pblaszko, marcinszkudlinski, lgirdwood, plbossart, mmaka1 and lbetlej as code owners January 20, 2025 21:35

ujfalusi reviewed Jan 21, 2025

View reviewed changes

src/audio/base_fw.c Outdated Show resolved Hide resolved

ujfalusi reviewed Jan 21, 2025

View reviewed changes

src/audio/base_fw.c Outdated Show resolved Hide resolved

lyakh reviewed Jan 21, 2025

View reviewed changes

lgirdwood reviewed Jan 21, 2025

View reviewed changes

jsarha reviewed Jan 22, 2025

View reviewed changes

src/audio/copier/copier_dai.c Outdated Show resolved Hide resolved

src/lib/dma.c Outdated Show resolved Hide resolved

src/include/ipc4/base_fw.h Outdated Show resolved Hide resolved

abonislawski force-pushed the mbukowsk-mic_privacy branch 6 times, most recently from 70c6211 to 37336ba Compare February 8, 2025 16:32

lyakh reviewed Feb 10, 2025

View reviewed changes

src/audio/copier/copier.c Outdated Show resolved Hide resolved

lgirdwood approved these changes Feb 10, 2025

View reviewed changes

ujfalusi requested changes Feb 10, 2025

View reviewed changes

ujfalusi reviewed Feb 11, 2025

View reviewed changes

mbukowsk added 4 commits February 11, 2025 09:11

ipc: added IPC SET_MIC_PRIVACY_FW_MANAGED_POLICY_MAS

4f48162

Added empty implementation which always returns success Signed-off-by: Michal Bukowski <[email protected]>

mic_privacy: implement new IPC - SNDW_MIC_PRIVACY_HW_MANAGED_STATE_CH…

e5a1ec1

…ANGE SW sends this IPC when microphone privacy state is changed for HW_MANAGED mode and SNDW interface. Signed-off-by: Michal Bukowski <[email protected]>

abonislawski force-pushed the mbukowsk-mic_privacy branch from 37336ba to e5a1ec1 Compare February 11, 2025 11:30

ujfalusi reviewed Feb 11, 2025

View reviewed changes

lgirdwood reviewed Feb 11, 2025

View reviewed changes


		uint32_t get_dma_zeroing_wait_time(void);

		uint32_t get_privacy_mask(void);


		LOG_MODULE_REGISTER(LOG_DOMAIN);

		int mic_privacy_manager_init(void)

mic_privacy: initial implementation #9788

Are you sure you want to change the base?

mic_privacy: initial implementation #9788

Conversation

mbukowsk commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsarha left a comment • edited Loading

Choose a reason for hiding this comment

lgirdwood commented Feb 10, 2025

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Feb 10, 2025

ujfalusi commented Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

jsarha left a comment •

edited

Loading

ujfalusi commented Feb 11, 2025 •

edited

Loading